Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge pull request #51 #53

Merged
merged 34 commits into from
Jun 5, 2020
Merged

Merge pull request #51 #53

merged 34 commits into from
Jun 5, 2020

Conversation

qindazhu
Copy link
Collaborator

@qindazhu qindazhu commented Jun 5, 2020

This PR is totally from Dan's PR https://github.com/danpovey/k2/pull/51 about Array2 , I just fix a few style issues.

@danpovey
Copy link
Collaborator

danpovey commented Jun 5, 2020 via email

@qindazhu
Copy link
Collaborator Author

qindazhu commented Jun 5, 2020

Merging as previous comments in pull request https://github.com/danpovey/k2/pull/51 have been fixed. We can fix any other issues later while we use Array in other FSA algorithms.

@qindazhu qindazhu merged commit 071b0b3 into k2-fsa:master Jun 5, 2020
@qindazhu qindazhu deleted the haowen-array branch June 5, 2020 09:01
@qindazhu qindazhu mentioned this pull request Jun 5, 2020
# would be (150,None,None).
# The Nones are for dimensions where the shape is not known
# because it is variable.
return (indexes.shape[0] - 1, None, *data.shape[1:])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comments are confusing compared to
return (indexes.shape[0] - 1, None, *data.shape[1:]).

Could you please make it clearer? @danpovey

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think indexes.shape should be 16 here. Can you please edit though? Sorry I am a bit busy today. I am hoping @qindazhu can have a look at your PR. Thanks so much!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants